Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: check paths completed for all retrieval types #386

Closed
wants to merge 21 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Aug 18, 2023

Depends on filecoin-project/go-data-transfer#378 for the graphsync part of this. Also includes a little bit of cleanup that makes the diff noisier.

Because traversals don't strictly need to run to any pre-defined completion, we have to check it ourselves - we watch the LastPath in the traversal progress and then check whether it at least fulfils our initial path request.

@codecov-commenter
Copy link

Codecov Report

Merging #386 (79b6f80) into rvagg/byte-range-proper (20a1138) will increase coverage by 0.05%.
The diff coverage is 92.66%.

Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                     @@
##           rvagg/byte-range-proper     #386      +/-   ##
===========================================================
+ Coverage                    76.93%   76.98%   +0.05%     
===========================================================
  Files                           87       87              
  Lines                         6611     6608       -3     
===========================================================
+ Hits                          5086     5087       +1     
+ Misses                        1255     1254       -1     
+ Partials                       270      267       -3     
Files Changed Coverage Δ
pkg/lassie/lassie.go 73.94% <ø> (ø)
pkg/retriever/retriever.go 89.51% <ø> (ø)
pkg/verifiedcar/verifiedcar.go 84.57% <88.40%> (+3.94%) ⬆️
pkg/internal/testutil/mockclient.go 55.55% <100.00%> (+1.20%) ⬆️
pkg/retriever/bitswapretriever.go 95.65% <100.00%> (+1.68%) ⬆️
pkg/retriever/graphsync/client/client.go 68.01% <100.00%> (ø)
pkg/retriever/graphsyncretriever.go 86.41% <100.00%> (+0.70%) ⬆️
pkg/retriever/httpretriever.go 85.83% <100.00%> (+0.11%) ⬆️
pkg/retriever/parallelpeerretriever.go 93.36% <100.00%> (-0.89%) ⬇️
pkg/storage/duplicateaddercar.go 90.43% <100.00%> (-1.53%) ⬇️

... and 4 files with indirect coverage changes

@rvagg rvagg marked this pull request as draft August 22, 2023 00:35
@rvagg
Copy link
Member Author

rvagg commented Aug 22, 2023

Changed to Draft while we consider strategy - maybe via the daemon you don't need/want to signal an error as you technically fulfilled the request and it's up to a upstream party to decide that this is an error. On the CLI we probably want to let the user know.

Also, maybe the error shouldn't be per retriever, perhaps it ought to be validated after the retrieval occurs--in which case maybe the RetrievalStats should include a LastPath from the traversal so we can easily verify it outside of the process?

@rvagg
Copy link
Member Author

rvagg commented Sep 6, 2023

some of this will be included in #404, we can opt for checking lassie fetch retrievals and reporting to the user when we have that functionality available, it'll just be a matter of wiring up the pieces

@rvagg rvagg closed this Sep 6, 2023
@rvagg rvagg deleted the rvagg/path-too-far-fail branch September 6, 2023 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants